-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bitcoin native experience #371
Merged
Merged
+1,077
−551
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To build Bitcoin-native experience.
Pass only bitcoin address to initialize the deposit process. Using `@orangekit/sdk` we calculate the Ethereum address based on the Bitcoin address. This created Ethereum address of the Safe should be used to show the user's data like position, transaction history and activity details.
✅ Deploy Preview for acre-dapp-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
nkuba
reviewed
Apr 22, 2024
To get the bitcoin address for `x'/0'/0'/0/0` path of a given extended public key to use always the same bitocin address as an identifier because Leder Wallet API returns the "next" public address where a user should receive funds. In the context of Bitcoin, the address is "renewed" each time funds are received. We need to use the same bitcoin address to generate always the same deposit owner address on ethereum.
We want to define an interface for communication with Bitcoin wallet and use it in Acre SDK. Currently it defines `getAddress` function because we need the bitcoin address to initialize the deposit.
Implements the `getAddress` function which always returns the same bitcoin address based on the extended public key.
Use mixin pattern to share abstract logic but for different base classes.
We want to relay on the `BitcoinProvider` implementation to get the bitcoin address because different wallets use different strategies. For example in Ledger Live the address is "renewed" each time funds are received in order to allow some privacy. In that case always getting the same bitcoin address is hidden under a given implementation.
Cover a case when the custom bitcoin recovery address is passed to initialize deposit.
Explain in the docs that this property is available to let the consumer use `P2SH-P2WPKH` as the deposit owner and another tBTC-supported type (`P2WPKH`, `P2PKH`) address as the tBTC Bridge recovery address.
`depositor` -> `depositorOwnerBitcoinAddress` - let's avoid `naming it `depositor` so we don't confuse it with the `BitcoinDepositor` contract.
According to the [jest documentation](https://jestjs.io/docs/getting-started#using-typescript) there are two ways to get TypeScript to work with Jest: - babel - ts-jest We already use ts-jest preset in jest configuration, so we don't need to import babel presets.
We use the JestConfigWithTsJest type to be explicit about the type of the config object we're dealing with.
After we added @orangekit/sdk import Jest started to complain with: ``` Jest encountered an unexpected token Details: /Users/jakub/workspace/acre/acre/node_modules/.pnpm/@orangekit[email protected]/node_modules/@orangekit/sdk/dist/src/index.js:1 ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){export * from "./lib"; ^^^^^^ SyntaxError: Unexpected token 'export' > 1 | import { OrangeKitSdk } from "@orangekit/sdk" | ^ 2 | import { JsonRpcProvider } from "ethers" 3 | import { AcreContracts } from "./lib/contracts" 4 | import { EthereumNetwork, getEthereumContracts } from "./lib/ethereum" at Runtime.createScriptFromCode (../node_modules/.pnpm/[email protected]/node_modules/jest-runtime/build/index.js:1505:14) at Object.<anonymous> (src/acre.ts:1:1) at Object.<anonymous> (src/index.ts:9:1) at Object.<anonymous> (test/modules/tbtc/Tbtc.test.ts:7:1) ``` The `@orangekit/sdk` module exports `dist/` directory containing JS files in ESM syntax. Jest doesn't support ESM syntax, so we have to convert these JS files to CommonJS syntax with `ts-jest/presets/js-with-ts` preset. We have to define `transformIgnorePatterns` property as `node_modules/` directory is excluded from transformations by default. We also had to add `allowJs: true` in tsconfig.json to support transformation of JS files. Reference: - https://stackoverflow.com/questions/75452411/why-isnt-jest-handling-es-module-dependencies - https://stackoverflow.com/questions/49263429/jest-gives-an-error-syntaxerror-unexpected-token-export?noredirect=1&lq=1
Depends on: #417 After we added @orangekit/sdk import Jest started to complain with: ``` Jest encountered an unexpected token Details: /Users/jakub/workspace/acre/acre/node_modules/.pnpm/@orangekit[email protected]/node_modules/@orangekit/sdk/dist/src/index.js:1 ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){export * from "./lib"; ^^^^^^ SyntaxError: Unexpected token 'export' > 1 | import { OrangeKitSdk } from "@orangekit/sdk" | ^ 2 | import { JsonRpcProvider } from "ethers" 3 | import { AcreContracts } from "./lib/contracts" 4 | import { EthereumNetwork, getEthereumContracts } from "./lib/ethereum" at Runtime.createScriptFromCode (../node_modules/.pnpm/[email protected]/node_modules/jest-runtime/build/index.js:1505:14) at Object.<anonymous> (src/acre.ts:1:1) at Object.<anonymous> (src/index.ts:9:1) at Object.<anonymous> (test/modules/tbtc/Tbtc.test.ts:7:1) ``` The `@orangekit/sdk` module exports `dist/` directory containing JS files in ESM syntax. Jest doesn't support ESM syntax, so we have to convert these JS files to CommonJS syntax with `ts-jest/presets/js-with-ts` preset. We have to define `transformIgnorePatterns` property as `node_modules/` directory is excluded from transformations by default. We also had to add `allowJs: true` in tsconfig.json to support transformation of JS files. Unfortunately, it increased test execution time, so it may be better to mock the `@orangekit/sdk` in unit tests instead of transforming the code. Reference: - https://stackoverflow.com/questions/75452411/why-isnt-jest-handling-es-module-dependencies - https://stackoverflow.com/questions/49263429/jest-gives-an-error-syntaxerror-unexpected-token-export?noredirect=1&lq=1
nkuba
reviewed
May 13, 2024
For security reasons. In the future, we should probably fork it or copy that we need to resolve the addresses from `xpub`.
We should avoid prefixing constructor args with `_` as it may suggest that the argument should be ignored, but we use it.
Define two separate functions `initializeMainnet` and `initializeTestnet` so consumers won't pass the network param unnecessarily.
Create `VoidSigner` compatible with ethers v5. In the future we are going to use the ethers signer from `@orangekit/sdk` under the hood. This signer is used only interanlly by the Acre SDK to interact with Ethereum contracts.
If there is one export we should use it as default.
Reverts 553b5be commit. We can just mock the `@orangekit/sdk` module globally to run jest tests correctly w/o converting the JS files to CommonJS syntax.
Depends on: #371 This PR removes the Ethereum account from the dapp context to implement Bitcoin native experience. Since the signing Bitcoin messages doesn't work in the Ledger Live Wallet API we temporarily removed this step from the deposit flow.
Remove unnecessary `ETHEREUM_NETWORK` param. The ethereum network is no longer needed to create the Acre SDK.
nkuba
reviewed
May 15, 2024
Merged
`ethereumAddress` -> `depositOwnerEvmAddress`
Fix docs for the `bitcoinProvider` property.
Extends the `Signer` interface from ethers v6 to include more methods required by ethers v5.
This was referenced May 17, 2024
nkuba
reviewed
May 29, 2024
Instead of exporting the BitcoinNetwork we import from tBTC SDK we define it with the networks we support - mainnet and testnet.
Remove the `initializeMainnet` and `initializeTestnet` static functions and keep only one function `initialize` and the consumer must pass bitcoin network to which it wants to connect. This simplifies the initialization in the dapp.
To improve the readability.
This is just a temporary interface that should be replaced by the `OrangeKitBitcoinWalletProvider` from the `orangekit` library.
nkuba
approved these changes
May 29, 2024
kkosiorowska
added a commit
that referenced
this pull request
May 29, 2024
Depends on: #371 This PR adds support for fetching the deposits in SDK. We must fetch data from both sources tbtc API and Acre subgraph because the tbtc API includes additional `queued` deposits - this means they have not yet been initialized or finalized. The subgraph only indexes initialized and finalized deposits, so we need to combine them.
r-czajkowski
added a commit
that referenced
this pull request
May 29, 2024
… flow (#434) Depends on: #371 This PR removes the unnecessary check condition if the `ethAccount` exists for deposit flow. The ETH account is no longer needed, so we should remove it from the wallet context. However, let's do it in a separate PR. ### Implemented solution https://github.com/thesis/acre/assets/23117945/ddec8803-c986-47bc-977d-eed11bf2a069
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #351
This PR integrates with the OrangeKit SDK to predict the depositor owner Ethereum address based on user's Bitcoin address. We want to transition to Bitcoin native experience in the SDK as well so we should operate on the bitcoin addresses only and hide the Ethereum integration under the hood. In the future, we will update the other function parameters to accept Bitcoin addresses instead of Ethereum.
Here we also introduce the
BitcoinProvider
interface which defines thegetAddress
method. We should rely on theBitcoinProvider
implementation to get the Bitcoin address because different wallets use different strategies. For example, in Ledger Live the address is "renewed" each time funds are received in order to allow some privacy. In this case, relying only on the Bitcoin address, the user would create deposits for different Ethereum addresses each time the Bitcoin address was renewed.Therefore, we implement the
BitcoinProvider
for Ledger Live Wallet API that it awalys returns the same Bitcoin address based on the extended public key (xpub
). The Ledger Live Wallet API increments theindex
in the derivation path each time funds are received(m/x'/0'/0'/0/0
;m/x'/0'/0'/0/1
;m/x'/0'/0'/0/2
...), so to get the same address we need to derive a single address from a given extended public key - an address under them/x'/0'/0'/0/0
path. Thanks to this we can create a deposit for the same Ethereum address even if Ledger Live renewed the Bitcoin address.